-
Notifications
You must be signed in to change notification settings - Fork 308
fix(insights): remove suspense from SearchButton #2957
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
cprussin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure but I think this will cause more perf issues because it means that all pages will have to delay for the async stuff in <SearchButton> before rendering the layout.
@alexcambose I suggest you first merge your perf improvements PR, then rebase this on top and do some testing to ensure it doesn't cause more perf issues.
We might have to think of another way to implement the global search if we can't use suspense.
|
@cprussin I agree. Do you think it would make sense to statically fetch/generate the data at build time and then inject it into these components? Wondering how often things will change in relation to the search. |
|
Tested some more and somehow it doesn't seem to add any additional latency even without Suspense. |
lol ok, that's surprising but maybe just due to the added redis caching... let's ship it! |
|
This is one of the "I have no idea how it works but it works" |
Summary
Removed Suspense from extraCta to fix server boundary crash in header. This does fix the error but it's not super optimal as we're loosing the
<SearchButtonImpl isLoading />state.Rationale
I think the issue is that Nextjs can’t stream Suspense data inside props passed to a Client Component;
And it throws
The server could not finish this Suspense boundary, likely due to an error during server rendering. Switched to client rendering.Another possible way to fix this would be to just fetch the data on the client side with useState/useEffect.
Tried a few other ways, but couldn't really get it to either work or not throw the same error.
How has this been tested?